Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: WIP GNO mail (no render) #641

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

grepsuzette
Copy link
Contributor

A mail system for GNO, that can be added to demo/ for now.

The low-level is functional, but there is no UI (Render() is "TODO").
Contributions welcome :)

Also as you can see in the notes, I have my doubt if this shouldn't be a more generic system.
Yep. Let's go!

@grepsuzette grepsuzette requested a review from a team as a code owner March 24, 2023 08:36
@harry-hov
Copy link
Contributor

Thank you for taking the time to contribute 🙏 . I left some initial review comments. Will do a detailed review later.

Please consider running make fmt.

@grepsuzette
Copy link
Contributor Author

Thank you for these initial feedbacks Harry 🙏 . Commited the suggested changes and made sure test passed.
Awaiting further review.

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution 🙏

I like the idea, it's interesting to have message system like this available for realms and packages.

I have left comments that need addressing first, after which I'll do another round of reviews

Comment on lines 3 to 19
import (
"gno.land/p/demo/avl"
"gno.land/p/demo/releases"
"std"
)

// realm state
var (
addr2Mailbox = avl.Tree{} // Address -> *Mailbox
counter int = 0 // number of mails ever sent
admin std.Address = "g1fjh9y7ausp27dqsdq0qrcsnmgvwm6829v2au7d" // @grepsuzette

Fee std.Coin = std.Coin{
Denom: "ugnot",
Amount: 250 * 1000, // should never cost more $0.12 or 0.25
}
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should be separating out state from the base file where that state is actively being used - it just creates confusion on where it's coming from. What do you think about moving this snippet closer to where it's being used?

Copy link
Contributor Author

@grepsuzette grepsuzette Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I certainly lack experience here, I see Thomas agrees with you.
I'll let you know my thought to have a chance to understand why you disagree. For me:

  • State is the most central thing for a realm.
  • The code gravitates around it and is interchangeable.
  • Therefore knowing what is state is key to approaching a realm.

Approaching one realm, I always look for the part where the state is defined.
The most obvious label for me for the state is to look for a file called state.gno.
This way it's possible to open state.gno in one split and work on something else.
If a folder doesn't contain one, I will respect the author choice, but then I will have to open a lot of files until I find it.

But in fact, I don't even argue to have files called a certain way, I argue authors should have their own style; I think diversity is good for the project, like in evolution. We don't really know what is best at this point IMO.

examples/gno.land/r/demo/mail/predicates.gno Show resolved Hide resolved
)

type Mail struct {
id int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should be using an int for a Mail ID - considering this can be something that is potentially used a lot as a message passing realm. Message IDs should probably be something derived from the mail message itself, rather than a counter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Me neither.
But if int in gno means int64, and since 2^63 is 9,223,372,036,854,775,808.
Let's say the system is so popular it generates one million messages per seconds.
It would take 92233720368 seconds to overflow, that is 2924 years.
While it's not perfect (one source claims more than 3 millions email were sent per seconds in 2023) I think this is a bigger discussion.

  • hash(msg): not good IMO (collisions will be trivial to find in the future),
  • the transaction number that produced the message may seem good, except when the system can send a message to several recipients, it will need a suffix to work.

As it's WIP and we have staging testnets I think we can keep an int for the time being.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use int for now and later switch to bigint (#764), maybe.

@@ -0,0 +1,42 @@
package mail
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about moving this Mail library logic to examples/gno.land/p/demo/mail, instead of having it sit in a Realm?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Milos!

Thanks taking the time to review this. Yes, deciding what to put in p/ and r/ has been one interrogation of mine.

Here are my thoughts, here is my starting point:

  • p is package, is like library, is common code for everyone to use, meaning it's quite stable.
  • r is realm, is app-like, meaning usage is restricted.

By that logic, code that isn't reasonnably finalized at least during GoR is better kept in r/ as it won't break anything (because nobody will be using it as a dependency).

I think putting things in p/demo is a signal to other devs saying: this code is stable, well-thought out please use it. My app is not there yet, it's WIP.

Another question I have with this app is:

Whether the functionality it offers is really about mails? or about generic messages.
If it's the latter, then the part to put in p/ is not mail, it would be like p/demo/msg, with r/demo/mail being a realm (almost an app instance) using that system.

This tells me it's a bit early to make the separation now.

@grepsuzette
Copy link
Contributor Author

tests don't pass, please wait.

@moul moul marked this pull request as draft May 4, 2023 05:48
@ajnavarro
Copy link
Contributor

ajnavarro commented Jun 1, 2023

Hey @grepsuzette !

Do you need any help to move this mail realm forward? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Status: 🌟 Wanted for Launch
Development

Successfully merging this pull request may close these issues.

6 participants